Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: user auth #47

Merged
merged 22 commits into from
Oct 4, 2023
Merged

feat: user auth #47

merged 22 commits into from
Oct 4, 2023

Conversation

Hajbo
Copy link
Collaborator

@Hajbo Hajbo commented Sep 27, 2023

Closes #30

db/config.ts Outdated Show resolved Hide resolved
db/migrations/migrate.ts Outdated Show resolved Hide resolved
db/seed.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@yamcodes
Copy link
Contributor

yamcodes commented Sep 28, 2023

This is looking interesting ^_^ Going to take a closer look tomorrow. Sorry for the early review before you were done, I didn't realize it was still a draft

@yamcodes yamcodes marked this pull request as draft September 28, 2023 00:07
@yamcodes yamcodes changed the title feature: user auth feat: user auth Sep 28, 2023
@Hajbo Hajbo marked this pull request as ready for review September 28, 2023 20:29
@Hajbo Hajbo requested a review from yamcodes September 28, 2023 20:29
@Hajbo Hajbo linked an issue Sep 28, 2023 that may be closed by this pull request
@yamcodes
Copy link
Contributor

yamcodes commented Oct 2, 2023

@Hajbo FYI, I changed the description of this PR from "Issue #30" to "Closes #30", following Linking a pull request to an issue using a keyword

yamcodes
yamcodes previously approved these changes Oct 2, 2023
Copy link
Contributor

@yamcodes yamcodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. I like how you handled the environment variables configuration using TypeBox. The usage is as simple as env.%myEnvironmentVariable% which is typesafe and validated. Lovely!

I also like the clear separation of concerns by placing the auth.ts in its own file.

Finally, I appreciate the error handling that you added as a bonus in this PR, and I think your approach for that is clean and sensible.

Questions

What is the motivation to not use the Elysia JWT Plugin?

Is it because it is cumbersome to set it up for our 3-tier architecture?

Next Steps

  1. Consider using env-schema to make the env validation even cleaner and more out of box. That library works well with typebox.
  2. Consider clarifying the responsibility of auth.ts by making it either one of auth.middleware.ts or auth.service.ts. We should strive to stay true to our separation of concerns approach to avoid leaking responsibilities in the future as much as possible and keep the codebase robust.
  3. See if we can make Swagger work or ditch it.
  4. There's quite a lot going on in the users.schema.ts file, which feels like a betrayal of the RealWorld concept. Sure, it works for a small project to put it all in one file, but in the spirit of a RealWorld application at scale I believe we should break it down into 2 or 3 granular pieces. We should consider putting the pgTable in a .model. or .entity. file, and potentially even move the types to their own files too. RealWorld example here.

src/config.ts Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/users/users.plugin.ts Outdated Show resolved Hide resolved
src/errors.ts Outdated Show resolved Hide resolved
src/app.module.ts Outdated Show resolved Hide resolved
@Hajbo
Copy link
Collaborator Author

Hajbo commented Oct 3, 2023

This looks really good. I like how you handled the environment variables configuration using TypeBox. The usage is as simple as env.%myEnvironmentVariable% which is typesafe and validated. Lovely!

I also like the clear separation of concerns by placing the auth.ts in its own file.

Finally, I appreciate the error handling that you added as a bonus in this PR, and I think your approach for that is clean and sensible.

Questions

What is the motivation to not use the Elysia JWT Plugin?

Is it because it is cumbersome to set it up for our 3-tier architecture?

Next Steps

  1. Consider using env-schema to make the env validation even cleaner and more out of box. That library works well with typebox.
  2. Consider clarifying the responsibility of auth.ts by making it either one of auth.middleware.ts or auth.service.ts. We should strive to stay true to our separation of concerns approach to avoid leaking responsibilities in the future as much as possible and keep the codebase robust.
  3. See if we can make Swagger work or ditch it.
  4. There's quite a lot going on in the users.schema.ts file, which feels like a betrayal of the RealWorld concept. Sure, it works for a small project to put it all in one file, but in the spirit of a RealWorld application at scale I believe we should break it down into 2 or 3 granular pieces. We should consider putting the pgTable in a .model. or .entity. file, and potentially even move the types to their own files too. RealWorld example here.

The JWT plugin only puts some wrapper object to the request handler, we can't access it in a state or decorate or beforeHandle, which kind of makes it pointless for us. Maybe we could create some kind of guard to use it, but I'm not sure if we should do it, it's much easier to directly use the Jose library (which the JWT plugin is using anyways).

The current auth.js is a bunch of utility functions, it's neither middleware nor service. I'm not sure what's the Nestjs mentality about them though

I wouldn't say that there are a lot going on, it's one place to prepare all the schemas/types for a user. They are super connected. Even if this file was 500 lines long they still are tightly coupled and IMHO should be together. Easier to maintain like this. The example you linked seems like a nightmare to work with with these 10 line files that contain nothing of value

@yamcodes
Copy link
Contributor

yamcodes commented Oct 3, 2023

it's much easier to directly use the Jose library (which the JWT plugin is using anyways).

Great. While I'm saddened we're not using all Elysia has to offer, in reality we have to make some sacrifices in the face of (1) realizing that Elysia still has types and features missing (2) implementing things under the framework layer.

It feels like Elysia is designed to be truly embraced by lighter applications, ones with less separation of concerns. Do you feel the same way?

The current auth.js is a bunch of utility functions, it's neither middleware nor service. I'm not sure what's the Nestjs mentality about them though

auth.js 😂 I hope I don't have to discuss JavaScript at least until Type Annotations reaches stage 3.

Let's solve that separately in #52, I'm sure we can find a simple solution for this.

I wouldn't say that there are a lot going on, it's one place to prepare all the schemas/types for a user. They are super connected. Even if this file was 500 lines long they still are tightly coupled and IMHO should be together. Easier to maintain like this. The example you linked seems like a nightmare to work with with these 10 line files that contain nothing of value

I understand where you're coming from, but I'm still quite concerned by putting tables and interfaces under *.schema.* because they're not schema. Can we please call the file *.model.ts?

@yamcodes yamcodes mentioned this pull request Oct 3, 2023
@Hajbo Hajbo merged commit 915f8cd into main Oct 4, 2023
2 checks passed
@Hajbo Hajbo deleted the feature/user-auth branch October 4, 2023 17:49
This was referenced Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement login and register
2 participants